Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

first draft of splitting NWSS signals #1946

Closed
wants to merge 18 commits into from
Closed

first draft of splitting NWSS signals #1946

wants to merge 18 commits into from

Conversation

dsweber2
Copy link
Contributor

Description

This splits the dataset based on the provider and normalization (not every pair is actually present), and adds the metric signals. The resulting signals are called:

  • pcr_conc_smoothed_CDC_VERILY_flow-population
  • detect_prop_15d_CDC_VERILY_flow-population
  • percentile_CDC_VERILY_flow-population
  • ptc_15d_CDC_VERILY_flow-population
  • pcr_conc_smoothed_CDC_VERILY_microbial
  • detect_prop_15d_CDC_VERILY_microbial
  • percentile_CDC_VERILY_microbial
  • ptc_15d_CDC_VERILY_microbial
  • pcr_conc_smoothed_NWSS_flow-population
  • detect_prop_15d_NWSS_flow-population
  • percentile_NWSS_flow-population
  • ptc_15d_NWSS_flow-population
  • pcr_conc_smoothed_NWSS_microbial
  • detect_prop_15d_NWSS_microbial
  • percentile_NWSS_microbial
  • ptc_15d_NWSS_microbial
  • pcr_conc_smoothed_WWS_microbial
  • detect_prop_15d_WWS_microbial
  • percentile_WWS_microbial
  • ptc_15d_WWS_microbial

Some of these can have negative values; for e.g. ptc_15d, the values are small enough that I expect these may actually be exponents. Still looking into why the concentration data has negative values, which are too large to make sense as exponents.

Fixes

  • Fix generate_weights in the case where some weights are negative, and added a test for it

@dsweber2 dsweber2 changed the title first draft of splitting signals first draft of splitting NWSS signals Feb 23, 2024
@dsweber2
Copy link
Contributor Author

dsweber2 commented Mar 11, 2024 •
edited
Loading

Minh confirmed that this runs on her machine and that the output looks reasonable. A couple of things to do before we merge this:

  • figure out the right cut-off for sig digits so that the noise is removed
  • seeing what values the formerly negative concentrations became
  • write docs similar to this PR

@dsweber2 dsweber2 force-pushed the splittingNWSSSignals branch 3 times, most recently from df5aa19 to e86e5fa Compare March 29, 2024 18:36
@dsweber2 dsweber2 requested a review from dshemetov April 16, 2024 18:40
@dshemetov dshemetov mentioned this pull request Apr 20, 2024
Merged
@dsweber2
Copy link
Contributor Author

Currently not passing because of the same update that made nssp tests fail. Once that's merged and this is rebased it will pass.

@nmdefries nmdefries requested review from nmdefries and removed request for melange396 and minhkhul June 6, 2024 16:27
nmdefries
nmdefries requested changes Jun 7, 2024
View reviewed changes
Copy link
Contributor

@nmdefries nmdefries left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments about style and comments. The actual functionality here looks fine.

This hasn't been released at all yet, right? You previously did statistical review on this; is there anything else we want to do for these new signals?

nwss_wastewater/delphi_nwss/constants.py Outdated Show resolved Hide resolved
nwss_wastewater/delphi_nwss/constants.py Show resolved Hide resolved
nwss_wastewater/delphi_nwss/pull.py Outdated Show resolved Hide resolved
nwss_wastewater/delphi_nwss/pull.py Outdated Show resolved Hide resolved
nwss_wastewater/delphi_nwss/pull.py Outdated Show resolved Hide resolved
nwss_wastewater/delphi_nwss/run.py
agg_df["geo_id"] = "us"
return agg_df


def add_needed_columns(df, col_names=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (optional): to make this more robust, add assert to make sure that our set of missing column names doesn't include important ones (like geo_id and value). Since this is out of scope, worth making an issue for

nwss_wastewater/delphi_nwss/run.py Outdated Show resolved Hide resolved
nwss_wastewater/delphi_nwss/run.py Outdated Show resolved Hide resolved
nwss_wastewater/delphi_nwss/run.py Outdated Show resolved Hide resolved
nwss_wastewater/delphi_nwss/run.py Outdated Show resolved Hide resolved
@dsweber2
Copy link
Contributor Author

dsweber2 commented Jun 7, 2024

This hasn't been released at all yet, right? You previously did statistical review on this; is there anything else we want to do for these new signals?

It has not. There's the corresponding docs, which I think Will read through at one point.

@nmdefries nmdefries self-requested a review June 25, 2024 16:41
@dsweber2
Copy link
Contributor Author

closing as this is migrating to a new endpoint

@dsweber2 dsweber2 closed this Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dshemetov dshemetov dshemetov approved these changes

@nmdefries nmdefries Awaiting requested review from nmdefries

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dsweber2 @dshemetov @nmdefries